-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mark final types as exact #88163
Mark final types as exact #88163
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsShould let the JIT devirtualize way more stuff due to knowing the exact types. Extracted from #87579.
|
/azp list |
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-extra-platforms, runtime-coreclr pgostress |
Azure Pipelines successfully started running 4 pipeline(s). |
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-extra-platforms, runtime-coreclr pgostress |
Azure Pipelines successfully started running 4 pipeline(s). |
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-extra-platforms, runtime-coreclr pgostress |
Azure Pipelines successfully started running 4 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we ought to put the calls to getExactClasses
into impIsClassExact
to centralize the logic, and then just have these methods simply call impIsClassExact
if they don't have exact classes.
As far as I know that's not needed because so the |
So (perhaps I'm finally understanding what you were saying above) the only change needed is to one call to @MichalPetryka can you please try just running with that one change? |
@MihuBot -hetzner |
The x86 failure here is a bit concerning. We are crashing in the GC.
|
I don't recall that being there before, could it maybe be the rebase on top of main that caused this? |
why is this acceptable? |
@dotnet/gc any thoughts on this? I can load up the dump but can't get SOS to work. |
We are expecting this change to unblock more devirtualization and inlining, so some size increases are expected. |
Also we expect the size increases in practice will be smaller since many of those same inlines are already happening under PGO-driven GDV. But we can't see that impact easily right now. Once this change is in and we do new SPMI collections we can undo it and get a more accurate read on the size impact. |
the test failure appears to be a GC hole, so might be useful to try with HeapVerify enabled to check if repros consistently. |
Let me see if it repros in CI. |
I can't seem to get rerun to actually rerun the failing test. @MichalPetryka please push a merge commit once #88856 is in so we can start from scratch. |
/azp run runtime-coreclr outerloop, runtime-extra-platforms, runtime-coreclr pgostress |
Azure Pipelines successfully started running 3 pipeline(s). |
Going to bounce this... |
Actually let me merge this up. |
@MichalPetryka thank you! |
Should let the JIT devirtualize way more stuff due to knowing the exact types.
Extracted from #87579.